AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed

What was reviewed

Skipped (out of scope): dist/, pnpm-lock.yaml, openspec/ artefacts

Findings

🔶 Medium — CI check failing: "Verify changelog entries" + lint warnings

File: .github/workflows / CI job Run code quality check
Problem: The Verify changelog entries step exits with code 1, failing the required CI check. Additionally, the CI run surfaced multiple ESLint import-order warnings across pre-existing files (not caused by this PR, but they gate the same job). The one annotation directly in this PR's test file is:

! Avoid calling `expect` conditionally
packages/pluggableWidgets/combobox-web/src/__tests__/SingleSelection.spec.tsx#284

This is the spinner test that wraps expect inside an if (spinner) guard:

// line ~280-284
if (spinner) {
    expect(spinner).not.toHaveAttribute("aria-hidden", "true");
}

If spinner is null the assertion is silently skipped — this is both a lint violation and a test-quality issue. Use an unconditional assertion or skip the test explicitly:

// Option A — assert it exists first
const spinner = component.container.querySelector(".widget-combobox-spinner");
expect(spinner).toBeInTheDocument();
expect(spinner).not.toHaveAttribute("aria-hidden", "true");

// Option B — if the spinner may not render, skip the test
// and add a note explaining the condition

Fix: Remove the conditional expect; if the spinner genuinely may not be present for this prop combination, rethink the test setup so the spinner is guaranteed to render, or add an assertion that it is absent by design.

🔶 Medium — getInputLabel called on every render via useMemo without DOM-query memoization

File: packages/pluggableWidgets/combobox-web/src/helpers/utils.ts line 153
packages/pluggableWidgets/combobox-web/src/components/SingleSelection/SingleSelection.tsx line 86
packages/pluggableWidgets/combobox-web/src/components/MultiSelection/MultiSelection.tsx line 100

Problem: getInputLabel(options.inputId) calls document.querySelector(...) on every render pass before the useMemo for ariaLabels sees the result. The returned Element | null changes reference each time querySelector finds the same DOM node (actually it returns the same node object, so this is stable, but the call happens on every render). More critically, inputLabel is not in the useMemo dependency array of ariaLabels:

// SingleSelection.tsx line 88-98
const ariaLabels = useMemo(
    () => getComboboxAriaLabels({ ..., inputLabel, ... }),
    [selectedItem, inputLabel, ariaLabelledBy, options.ariaLabel, selector.caption]
);

inputLabel is listed as a dep here — that's correct. But getInputLabel is called on every render outside any memo, making the dep tracking fragile: if the label element is added/removed from the DOM after mount (e.g., conditional rendering of the label), the memo will never re-run because inputLabel always references the same DOM node object (identity equality). This was a pre-existing issue but the PR doubles down on it by calling getInputLabel later in the component body (after the Downshift hooks) — which is fine for correctness today but worth noting.

Fix: This is acceptable as-is for now since labels are not expected to be conditionally mounted, but document the assumption in a comment, or use a useRef/useEffect to observe label presence if dynamic labels ever become a requirement. No blocking change needed.

⚠️ Low — alwaysOpen mode: header/footer now gated on isOpen, but alwaysOpen never sets isOpen=true

File: packages/pluggableWidgets/combobox-web/src/components/ComboboxMenuWrapper.tsx lines 77, 86, 98
Note: In the previous code, menuHeaderContent and menuFooterContent rendered regardless of isOpen. Now they're gated on {isOpen && ...}. When alwaysOpen={true} is passed, the outer container always renders with display: block, but isOpen could still be false — the menu would be visually shown but the header, <ul>, and footer would all be absent. Check whether alwaysOpen callers (editor preview) always also pass isOpen={true}, or guard it with {(isOpen || alwaysOpen) && ...}.

⚠️ Low — aria-label set simultaneously with Downshift-managed aria-labelledby when hasSelection=true and a visible label exists

File: packages/pluggableWidgets/combobox-web/src/helpers/utils.ts lines 189-193
Note: When hasSelection=true and hasLabel=true, ariaLabel is set to "<labelText>, <selectedValue>" and ariaLabelledBy is set to undefined. This means the visible <label for="..."> element in the DOM is disconnected from the input while a selection is active — screen readers will use the aria-label string rather than reading from the live label element. While this achieves the intended announcement, it creates a discrepancy between the DOM association and the programmatic name. Consider whether aria-describedby for the selected value might be a more idiomatic pattern (label names the field, describedby conveys current state), though the chosen approach is not a WCAG violation.

⚠️ Low — E2E test uses not.toBeVisible() instead of not.toBeInTheDocument() for DOM-removal assertion

File: packages/pluggableWidgets/combobox-web/e2e/Combobox.spec.js line 181
Note: The test comment says "Verify menu list is not in DOM when closed" but the assertion is:

await expect(menuListClosed).not.toBeVisible();

not.toBeVisible() passes when the element is hidden or absent. Since the intent of this change is that the element is not rendered at all, the assertion should be:

await expect(menuListClosed).toHaveCount(0);

This makes the test actually verify the DOM-removal guarantee rather than just visibility.

Positives

• Calling getMenuProps() unconditionally (moved outside the conditional {isOpen && ...} block) correctly satisfies Downshift's requirement that all prop-getter hooks are called on every render — this is the right fix for avoiding Downshift warnings when the menu is conditionally rendered.
• The getComboboxAriaLabels utility is well-documented with a clear comment explaining the aria-labelledby precedence rationale — this non-obvious ARIA behaviour is captured at the right level.
• New unit tests cover all specified scenarios (closed state, open with items, open empty, validation attributes) and correctly use waitFor for async DOM updates.
• aria-busy addition on the input during loading state is a nice ARIA improvement that aids screen readers in knowing when results are being fetched.
• The CHANGELOG entry is present and correctly scoped to user-observable behaviour (no implementation details).